From a55f36211efe1bb0d6717c8545366120bd6dfd9f Mon Sep 17 00:00:00 2001
From: Thiago Macieira <thiago.macieira@intel.com>
Date: Mon, 21 Nov 2016 15:17:03 -0800
Subject: Fix the JPEG EXIF reader to deal with some broken/corrupt files

We parse the EXIF header in order to get the proper orientation, so
let's be a bit more careful in what we accept. This patch adds better
handling for reading past the end of the stream, plus it limits the
number of IFDs read (to avoid processing too much data) and deals with a
pathological case of the EXIF file format: EXIF (due to its TIFF
origins) permits the offset to the next IFD to be backwards in the file,
which means it could result in a loop or pointing to plain corrupt data.
We disallow any backwards pointers, since it seems that's what other
decoders do (libexif, for example).

Change-Id: Iaeecaffe26af4535b416fffd1489332db92e3888
(cherry picked from 5.6 commit 02150649f95b8f46f826e6e002be3fa0b6d009bc)
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>
---
 src/gui/image/qjpeghandler.cpp                     |  31 ++++++++++++++-------
 .../jpeg_exif_invalid_data_back_pointers.jpg       | Bin 0 -> 910 bytes
 .../images/jpeg_exif_invalid_data_past_end.jpg     | Bin 0 -> 910 bytes
 .../jpeg_exif_invalid_data_too_many_ifds.jpg       | Bin 0 -> 964 bytes
 .../jpeg_exif_invalid_data_too_many_tags.jpg       | Bin 0 -> 910 bytes
 tests/auto/gui/image/qimage/tst_qimage.cpp         |  17 +++++++++--
 6 files changed, 35 insertions(+), 13 deletions(-)
 create mode 100644 tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_back_pointers.jpg
 create mode 100644 tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_past_end.jpg
 create mode 100644 tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_too_many_ifds.jpg
 create mode 100644 tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_too_many_tags.jpg

diff --git a/src/gui/image/qjpeghandler.cpp b/src/gui/image/qjpeghandler.cpp
index 52e8b39..f3ba303 100644
--- a/src/gui/image/qjpeghandler.cpp
+++ b/src/gui/image/qjpeghandler.cpp
@@ -786,6 +786,10 @@ static bool readExifHeader(QDataStream &stream)
  */
 static int getExifOrientation(QByteArray &exifData)
 {
+    // Current EXIF version (2.3) says there can be at most 5 IFDs,
+    // byte we allow for 10 so we're able to deal with future extensions.
+    const int maxIfdCount = 10;
+
     QDataStream stream(&exifData, QIODevice::ReadOnly);
 
     if (!readExifHeader(stream))
@@ -793,7 +797,8 @@ static int getExifOrientation(QByteArray &exifData)
 
     quint16 val;
     quint32 offset;
-    const qint64 headerStart = stream.device()->pos();
+    const qint64 headerStart = 6;   // the EXIF header has a constant size
+    Q_ASSERT(headerStart == stream.device()->pos());
 
     // read byte order marker
     stream >> val;
@@ -804,7 +809,7 @@ static int getExifOrientation(QByteArray &exifData)
     else
         return -1; // unknown byte order
 
-    // read size
+    // confirm byte order
     stream >> val;
     if (val != 0x2a)
         return -1;
@@ -812,18 +817,22 @@ static int getExifOrientation(QByteArray &exifData)
     stream >> offset;
 
     // read IFD
-    while (!stream.atEnd()) {
+    for (int n = 0; n < maxIfdCount; ++n) {
         quint16 numEntries;
 
-        // skip offset bytes to get the next IFD
         const qint64 bytesToSkip = offset - (stream.device()->pos() - headerStart);
-
-        if (stream.skipRawData(bytesToSkip) != bytesToSkip)
+        if (bytesToSkip < 0 || (offset + headerStart >= exifData.size())) {
+            // disallow going backwards, though it's permitted in the spec
             return -1;
+        } else if (bytesToSkip != 0) {
+            // seek to the IFD
+            if (!stream.device()->seek(offset + headerStart))
+                return -1;
+        }
 
         stream >> numEntries;
 
-        for (; numEntries > 0; --numEntries) {
+        for (; numEntries > 0 && stream.status() == QDataStream::Ok; --numEntries) {
             quint16 tag;
             quint16 type;
             quint32 components;
@@ -847,12 +856,14 @@ static int getExifOrientation(QByteArray &exifData)
 
         // read offset to next IFD
         stream >> offset;
+        if (stream.status() != QDataStream::Ok)
+            return -1;
         if (offset == 0) // this is the last IFD
-            break;
+            return 0;   // No Exif orientation was found
     }
 
-    // No Exif orientation was found
-    return 0;
+    // too many IFDs
+    return -1;
 }
 
 static QImageIOHandler::Transformations exif2Qt(int exifOrientation)
diff --git a/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_back_pointers.jpg b/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_back_pointers.jpg
new file mode 100644
index 0000000..164d308
Binary files /dev/null and b/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_back_pointers.jpg differ
diff --git a/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_past_end.jpg b/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_past_end.jpg
new file mode 100644
index 0000000..7e2451e
Binary files /dev/null and b/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_past_end.jpg differ
diff --git a/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_too_many_ifds.jpg b/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_too_many_ifds.jpg
new file mode 100644
index 0000000..52c6a93
Binary files /dev/null and b/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_too_many_ifds.jpg differ
diff --git a/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_too_many_tags.jpg b/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_too_many_tags.jpg
new file mode 100644
index 0000000..6a080aa
Binary files /dev/null and b/tests/auto/gui/image/qimage/images/jpeg_exif_invalid_data_too_many_tags.jpg differ
diff --git a/tests/auto/gui/image/qimage/tst_qimage.cpp b/tests/auto/gui/image/qimage/tst_qimage.cpp
index 85d258d..26772d5 100644
--- a/tests/auto/gui/image/qimage/tst_qimage.cpp
+++ b/tests/auto/gui/image/qimage/tst_qimage.cpp
@@ -183,7 +183,8 @@ private slots:
     void exifOrientation();
 
     void exif_QTBUG45865();
-    void exif_invalid_data_QTBUG46870();
+    void exifInvalidData_data();
+    void exifInvalidData();
 
     void cleanupFunctions();
 
@@ -3028,10 +3029,20 @@ void tst_QImage::exif_QTBUG45865()
     QCOMPARE(image.size(), QSize(5, 8));
 }
 
-void tst_QImage::exif_invalid_data_QTBUG46870()
+void tst_QImage::exifInvalidData_data()
+{
+    QTest::addColumn<bool>("$never used");
+    QTest::newRow("QTBUG-46870");
+    QTest::newRow("back_pointers");
+    QTest::newRow("past_end");
+    QTest::newRow("too_many_ifds");
+    QTest::newRow("too_many_tags");
+}
+
+void tst_QImage::exifInvalidData()
 {
     QImage image;
-    QVERIFY(image.load(m_prefix + "jpeg_exif_invalid_data_QTBUG-46870.jpg"));
+    QVERIFY(image.load(m_prefix + "jpeg_exif_invalid_data_" + QTest::currentDataTag() + ".jpg"));
     QVERIFY(!image.isNull());
 }
 
-- 
cgit v1.0-4-g1e03
 
